-
-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[16.0][MIG] stock_request_picking_type: Migration to 16.0 #21
[16.0][MIG] stock_request_picking_type: Migration to 16.0 #21
Conversation
[UPD] README.rst [UPD] Update stock_request_picking_type.pot [UPD] README.rst [UPD] Update stock_request_picking_type.pot
[UPD] Update stock_request_picking_type.pot [UPD] README.rst Added translation using Weblate (Chinese (Simplified)) Translated using Weblate (Chinese (Simplified)) Currently translated at 100.0% (28 of 28 strings) Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_request_picking_type Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_request_picking_type/zh_CN/ Added translation using Weblate (Spanish) Translated using Weblate (Spanish) Currently translated at 100.0% (28 of 28 strings) Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_request_picking_type Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_request_picking_type/es/
[UPD] README.rst [UPD] Update stock_request_picking_type.pot [UPD] README.rst [UPD] Update stock_request_picking_type.pot
[UPD] Update stock_request_picking_type.pot [UPD] README.rst Added translation using Weblate (Chinese (Simplified)) Translated using Weblate (Chinese (Simplified)) Currently translated at 100.0% (28 of 28 strings) Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_request_picking_type Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_request_picking_type/zh_CN/ Added translation using Weblate (Spanish) Translated using Weblate (Spanish) Currently translated at 100.0% (28 of 28 strings) Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_request_picking_type Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_request_picking_type/es/ Added translation using Weblate (Portuguese (Brazil)) Translated using Weblate (Portuguese (Brazil)) Currently translated at 100.0% (28 of 28 strings) Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_request_picking_type Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_request_picking_type/pt_BR/
[IMP] User write()
[IMP] Flake8
To appear in OCA shop in the good category.
[UPD] Update stock_request_picking_type.pot Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: stock-logistics-warehouse-13.0/stock-logistics-warehouse-13.0-stock_request_picking_type Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-13-0/stock-logistics-warehouse-13-0-stock_request_picking_type/
…ny editabe tree on sro request model
There's no issue in this repo with the title 'Migration to version 16.0' and the milestone 16.0, so not possible to add the comment. |
} | ||
) | ||
) | ||
|
||
def _create_product(self, default_code, name, company_id, **vals): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove those kind of helper methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -74,39 +90,11 @@ def setUpClass(cls): | |||
"stock.no_auto_scheduler", "True" | |||
) | |||
|
|||
def _create_user(self, name, group_ids, company_ids): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove those helper methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
cc31dd5
to
eee1a16
Compare
Fix it! Can review again please @rousseldenis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
self.env["res.users"] | ||
.with_context({"no_reset_password": True}) | ||
cls.env["res.users"] | ||
.with_context(**{"no_reset_password": True}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can avoid such writing by using arguments directly:
.with_context(no_reset_password=True)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix it!
@@ -185,4 +190,4 @@ def test_create(self): | |||
order = self.request_order.with_user(self.stock_request_user).create(order_vals) | |||
|
|||
# test create() | |||
self.assertEqual(order.picking_type_id, new_pick_type) | |||
self.assertNotEqual(order.picking_type_id, new_pick_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove the test because is not impacts in the total coverage at the module and now is not possible to modify the warehouse and location after creation in form.
eee1a16
to
50b061b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM tested functionally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review
picking_type_id = fields.Many2one( | ||
comodel_name="stock.picking.type", | ||
string="Operation Type", | ||
default=_get_default_picking_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deserves a lambda function instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: No more default if changed to compute.
|
||
@api.model | ||
def _get_default_picking_type(self): | ||
companies = self._context.get("allowed_company_ids", []).copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_context is deprecated since a long time: https://github.com/odoo/odoo/blob/16.0/odoo/models.py#L5160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still using here:
https://github.com/odoo/odoo/blob/17.0/odoo/models.py#L1158
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not why they are using it, we should do it too 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With self.env.context
much better no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's the one
[ | ||
("code", "=", "stock_request_order"), | ||
"|", | ||
("warehouse_id.company_id", "in", companies), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, I think this is not necessary as there is a record rule on picking type. I'm wondering why it is not sufficent to search on...
) | ||
|
||
@api.onchange("warehouse_id") | ||
def onchange_warehouse_picking_id(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be avoided. You can change the picking_type_id field to a computed/stored/readonly=False field instead and change this to _compute method.
self._origin.write({"picking_type_id": picking_type_id.id}) | ||
|
||
@api.model | ||
def create(self, vals): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be avoided too referring the remark above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create() override should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the field is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compute will do the job as its aim is to get a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compute is not the same as default because on create can't assign a value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use precompute=True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix it!
5ba5b3c
to
05cc872
Compare
All improvements done @rousseldenis |
required=True, | ||
store=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add readonly=False too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix it!
05cc872
to
6fc78fe
Compare
It's ready for merge @rousseldenis ? |
Any update @rousseldenis ? I need that for merge that module |
This PR has the |
/ocabot merge nobump |
On my way to merge this fine PR! |
Congratulations, your PR was merged at da59a7d. Thanks a lot for contributing to OCA. ❤️ |
Module migrated to version 16.0
cc https://github.com/APSL 153677
@miquelalzanillas @lbarry-apsl @javierobcn @mpascuall please review